Repository navigation
fix(agc): retain raw draw decodes across snapshot eviction - #10103
oneandonlydean merged 15 commits into
Conversation
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed the complete diff, the linked eviction report and the published qualification logs. The retained pointer path still goes through the same queue drain, GPU-write flush and exact mapped-byte comparison; the test covers eviction, changed main/tail code, restoration and an overlapping queued write. The report shows the intended test-only control failing on both GPU vendors and matched main/change runs with zero validation messages. The conventions check and git diff --check pass on this head, and the current pairwise merge is clean.
Please refresh this branch from current main and rerun the focused qualification before approval. This head is 59 commits behind and current main has since changed ShaderRegistry.cpp, DrawCache.cpp, GuestMemory and the two test files touched here. In particular, current main has removed the PreparedUnregisteredGraphics test code this PR modifies, and its shader snapshot and write-tracking paths now sit alongside substantial draw-pipeline and memory changes. The published GPU qualification is pinned to main fef15751e6 (with a source review of 1f1acb195e); it does not exercise the current 513ca7e558 integration. The pairwise merge is clean, but that alone does not validate these changed paths against the present implementation. After refreshing, please retain the eviction and invalidation checks in a test that runs on the current test path, then rerun the same focused main/head groups with the required validation layer on both GPUs and compare the logs. Also recheck the shared-file overlap with #9899 after the refresh; its feature is distinct, and I found no conflict in the current two-head merge.
|
@MarcZX10 Refreshed and requalified at The same focused main/change groups now pass on both RTX5080 and Radeon with the required loaded validation layer.47 positive processes (36 actualGPU,11 CPU/mock), plus2 intended failing controls from unchanged main production + identical regression tests. Zero validation warnings/errors/VUIDs, skips or timeouts; maximum2.329s. Native library aggregate/relinker and real selected fixtures build. Complete new logs, pins, source-blob comparison and advisory analysis. I checked the removed-test concern directly: the five PR-touched files are blob-identical between mainfef15751e6 and maine3d03fbae6 (also at the review's513ca7e558 pin). Current main retains Rechecked #9899 at9b4e692d70: the new source-only combined merge is clean, with no duplicate retained-snapshot implementation, combined build/test or agreed landing order claimed. AI-assisted implementation, validation and reporting: yes (Codex); author responsible. |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Re-reviewing exact head 31dc58ebb28372c5abc4b8ad900b7dc8003aafbc: the refreshed qualification resolves my previous blocker. I inspected the linked raw summary and manifest: literal main e3d03fbae64e1b49e5d4e84e753ccce6edf60da5 and this head each pass the same 18 GPU and 4 CPU/mock focused processes; the test-only control (unchanged main production plus the new tests) fails the intended eviction assertion on both RTX 5080 and Radeon, while queued-write checks pass. Required Vulkan validation was loaded and logs report no warnings, errors, VUIDs, skips or timeouts. The new test path explicitly executes the retained-decode check and covers eviction, changed main/tail code, restoration and queued writes.\n\nI fetched current main (af4700b98d981911af0c1809415db85c13127836). Since the qualification base, its commits change core/libs/CMakeLists.cpp, libSceAgc command tests and GNM Neo files; none of the five PR-touched implementation/test files changed. Current merge preview is clean; exact-head conventions and diff checks pass. Hosted conventions/labels/report pass; product jobs are skipped by policy. The PR does not claim a combined build with #9899, a full default suite, Linux/lavapipe, console equivalence, performance improvement or title unblock. No blocking issue remains.
|
Correction to the review text above: the unrelated project file is |
|
Potential test interaction with open #10174: that PR raises the raw snapshot limit to 4096 entries / 64 MiB. This PR’s 80-snapshot churn will then pass without evicting the held snapshot, so it will stop exercising the retained-decode-after-eviction behavior even though #10174 adds its own separate cache-eviction test. Could this regression churn enough distinct snapshots to force an eviction at the raised limit (or otherwise assert eviction occurred) when combining the changes? #10174 documents the same interaction. |
|
Fresh main refresh at Complete source pins, verbose logs and advisory analysis. Earlier main319 negative controls retain their original pins. No new title/performance/Linux/macOS or combined #9899 claim. A normal tested-main update retriggers automatic CI without changing its identity guard; the new hosted result remains pending. AI-assisted:yes(Codex). The fetchable |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Re-review of exact head 86a3a835eaf3293d0c05d7a30bc4d7fcaa1674d5: the revision addresses my eviction-coverage concern. The test now churns 4,097 distinct snapshots and explicitly requires a different snapshot identity before checking retained-decode validity, so it cannot pass without exercising eviction at the 4,096-entry capacity used by #10174. I reviewed the test and production diff: retained main/tail snapshots still pass through the existing ReadRawShader validation path, while the queued-write test checks invalidation with an outstanding overlapping write; changed main/tail contents and restoration are covered. The updated report separates standalone and #10174-composed pins and describes intended test-only control failures on both RTX 5080 and Radeon, with no Vulkan diagnostics/skips/timeouts. Current hosted conventions, labels, eligibility and report checks pass; product CI jobs are skipped by repository policy, so I rely on the author’s published Windows qualification rather than claiming hosted product CI ran. The PR identifies #9899 as independent and accurately says no landing order or combined qualification was agreed. I found no remaining blocking issue in this diff.
…fresh-pr10103-main9018-20261024
…fresh-pr10103-mainf188-20261027
|
Overlap with #11442 in #11442 is small:
Its review only asks for a sync with current Once we agree, I'll record the order with |
|
@Sam-mon7 Thanks; I will take your offered reverse order: #10103 -> #11442. This keeps the independently validated retained-decode correction moving while you carry the small helper/callee-read rebase afterward. Your drain helper and queued-callee tests remain your contribution. I checked the exact submitted heads One current-main observation for the reachability wording: This is source/landing-order review, not execution of the combined tree. AI-assisted: yes (Codex). |
✅ NVIDIA GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (4, not counted against this PR)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validation IDs already present on main (5 IDs in 476 tests)Skipped (44)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
@TechLevelZero, thank you for the NVIDIA/Linux run. I checked the complete supplied logs. Neither I rebuilt your exact main Your lazy-ABI test passes in 36.72 seconds, above the 30-second target. Our serial Windows main/change runs take 3.46–4.34 seconds, which does not resolve Linux timing. Could you provide a serial Linux main/change pair for |
|
Linux / AMD RDNA 4 (RADV) focused validation and serial timing run for head Host & GPU: CachyOS Linux ( Environment:
AI-assisted hardware test run (Antigravity). |
✅ AMD GPU test: passedPR head
Unstable on main (4, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (9, not counted against this PR)Validation IDs already present on main (5 IDs in 484 tests)Skipped (55)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
✅ NVIDIA GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validation IDs already present on main (5 IDs in 483 tests)Skipped (45)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
Shows the fix on RDNA 4 with AMD's Windows driver: Windows 11 Pro 10.0.22631, AMD Radeon RX 9070 XT (driver 32.0.32015.7029), MSYS2 UCRT64 g++ 16.1.0, Release,
The earlier Radeon runs here were at older heads or on RADV under Linux. This one is AMD's own Windows driver at the current head AI-assisted: yes (Claude Code) |
✅ NVIDIA GPU test: passedPR head
Unstable on main (3, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (2, not counted against this PR)Validation IDs already present on main (4 IDs in 517 tests)Skipped (44)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
What
Preserve a held raw draw decode when the global raw-shader snapshot cache evicts its immutable program snapshots.
DecodeProgramsCurrentcompares snapshot identity. A draw can retain a snapshot after the bounded global cache evicts it. #10174 is now merged, raising capacity to4,096 entries/64MiB; the4,097-snapshot regression still forces eviction. Rereading identical bytes after eviction creates a different snapshot and unnecessarily invalidates that decode. Supply its held main and tail snapshots toReadRawShader; an eligible retained snapshot uses the existing queued-write drain, GPU-write flush and exact byte comparison. Modified or unreadable code does not become a cache hit. The global cache limits and unsupported-path refusals are unchanged.The regression churns 4,097 other snapshots and requires eviction, checks retained decode validity after eviction, rejects changed main and tail words, and checks validity after restoring the original words. The queued-write regression checks that an overlapping outstanding write invalidates a retained decode. This changes snapshot reuse, not shader instruction semantics or rendered output.
Related #9899 addresses callee-link memoization and range-check relocation. It does not supply this correction. The overlap tool reports no conflicting lines at the recorded base; manual review covers the shared files of conflicting PRs. A source-only merge with #9899 is clean; that combined tree has not been built or tested. On26October the author confirmed that #9899 and this correction are independent and either can land first. No combined runtime qualification is claimed.
Queued-read overlap order: accepted the author's offered #10103 -> #11442 order. #11442 carries the downstream rebase: preserve this PR's retained-snapshot signature/selection and both pre-read drains when extracting its shared helper, then retain its additional callee/table drains. This is a merge-order agreement, not a functional prerequisite of #10103;
Depends on: noneremains. No combined build/GPU qualification is claimed. #9899 remains independently agreed/either order.Tested
2 November evening refresh: 76d2864 includes main 8fa9727, with published ancestry preserved. Exact paired source pins, commands, logs, artifact hashes, advisory analysis and limits.
Full configured native-library/relinker/actual-fixture build; 2 focused physical-GPU and 1 CPU passes. Conventions/whitespace pass. Common main checks and new capability tests are distinguished in the report. Earlier controls keep their original pins. No full-suite, new Linux/title/FPS or future combined-dependency claim.
Same-machine RTX 5080/NVIDIA 621.01 and Radeon Graphics/AMD 26.10.1, Windows 11 Enterprise 26200; actual devices and loaded Khronos 1.4.363 verified. Required Vulkan, cache off, verbose serial 30-second CTest. No selected validation warning/error/VUID or timeout. Device-limit skips, if any, are explicit in the report.
Independent RX 9070 XT Windows evidence: c9f7fe4/main4569 retained-decode test fails 5/5 restoring production sources and passes 5/5 with the fix; queued raw shader is CPU-only. Those source pins remain distinct from this refresh.
Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)